RSDK-9740: Make google libraries a private dependency of the SDK #357
Conversation
lia-viam
requested review from
njooma and
purplenicole730
and removed request for
a team
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
This is really great. I think several of the places where grpc is being tagged as a dependency are perhaps vestigial?
| target_link_libraries(simple_module | ||
| PRIVATE Threads::Threads | ||
| viam-cpp-sdk::viamsdk | ||
| ${VIAMCPPSDK_GRPCXX_LIBRARIES} |
There was a problem hiding this comment.
I'm a little surprised that simple_module has a direct link dependency on grpc?
| target_link_libraries(tflite_module | ||
| PRIVATE Threads::Threads | ||
| PRIVATE viam-cpp-sdk::viamsdk | ||
| PRIVATE ${VIAMCPPSDK_GRPCXX_LIBRARIES} |
There was a problem hiding this comment.
Ditto tflite module dependency on grpc
|
|
||
| target_link_libraries(example_module | ||
| viam-cpp-sdk::viamsdk | ||
| gRPC::grpc++ |
| PUBLIC Boost::log | ||
| PUBLIC xtensor | ||
| PRIVATE ${VIAMCPPSDK_GRPCXX_REFLECTION_LIBRARIES} | ||
| PRIVATE ${VIAMCPPSDK_GRPCXX_LIBRARIES} |
| PRIVATE ${VIAMCPPSDK_GRPCXX_REFLECTION_LIBRARIES} | ||
| PRIVATE ${VIAMCPPSDK_GRPCXX_LIBRARIES} | ||
| PRIVATE ${VIAMCPPSDK_GRPC_LIBRARIES} | ||
| PRIVATE ${VIAMCPPSDK_PROTOBUF_LIBRARIES} |
There was a problem hiding this comment.
⬇️ - I have a suspicion there may no longer be any actual direct usage of abseil in viamsdk. Can you please check?
There was a problem hiding this comment.
happily you're right!
| target_link_libraries(example_dial_api_key | ||
| Boost::program_options | ||
| viam-cpp-sdk::viamsdk | ||
| ${VIAMCPPSDK_GRPCXX_LIBRARIES} |
There was a problem hiding this comment.
I don't see any actual use of grpc in this examples sources.
| # | ||
| find_package(viam-cpp-sdk CONFIG REQUIRED viamsdk) | ||
|
|
||
| find_package(gRPC CONFIG REQUIRED) |
There was a problem hiding this comment.
Oops good catch thank you
|
|
||
| self.requires('xtensor/[>=0.24.3]') | ||
| self.requires('abseil/[>=20230125.3]', transitive_libs=True) | ||
| self.requires('abseil/[>=20230125.3]') |
There was a problem hiding this comment.
Is this actually still needed? I'd expect absl to be a transitive dep of proto/grpc so not needing to be mentioned here?
There was a problem hiding this comment.
I was surprised by this too but yes--my working theory is that it's because the API gencode does include some abseil headers/need some of their symbols, this was making the build fail with undefined symbol errors when I took it out
| if self.settings.os in ["Linux", "FreeBSD"]: | ||
| self.cpp_info.components[component].system_libs = ["pthread"] | ||
|
|
||
| self.cpp_info.components["viamapi"].requires.append("abseil::absl_strings") |
There was a problem hiding this comment.
Guessing this is related to above inclusion.
2 participants
No description provided.